fix(go): capture CLI stderr and fix SetProcessDone race#863
Open
claudiogodoy99 wants to merge 3 commits intogithub:mainfrom
Open
fix(go): capture CLI stderr and fix SetProcessDone race#863claudiogodoy99 wants to merge 3 commits intogithub:mainfrom
claudiogodoy99 wants to merge 3 commits intogithub:mainfrom
Conversation
Author
|
pr for issue: #862 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Go SDK’s error reporting and process-exit handling by removing a race in how the JSON-RPC client observes CLI process exit errors, and by capturing CLI stderr to enrich exit diagnostics.
Changes:
- Replace async copying of the CLI process error in
jsonrpc2.Clientwith a stored error pointer read afterprocessDonecloses. - Capture CLI
stderrinto a buffer and include it inmonitorProcess()exit errors. - Add unit tests covering the process error visibility and stderr capture behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| go/internal/jsonrpc2/jsonrpc2.go | Switches to pointer-based process error retrieval to avoid timing races after processDone closes. |
| go/internal/jsonrpc2/jsonrpc2_test.go | Adds tests for the process error visibility behavior (but some comments/names still describe the pre-change implementation). |
| go/client.go | Captures CLI stderr and appends it to process-exit errors. |
| go/client_test.go | Adds tests asserting stderr is included in the process error (but currently uses non-portable shell commands). |
Comment on lines
+723
to
+730
| // Verify that a bytes.Buffer assigned to Stderr is recognized by | ||
| // monitorProcess (type assertion to *bytes.Buffer). | ||
| cmd := exec.Command("true") | ||
| buf := &bytes.Buffer{} | ||
| cmd.Stderr = buf | ||
| if _, ok := cmd.Stderr.(*bytes.Buffer); !ok { | ||
| t.Error("expected Stderr to be *bytes.Buffer after assignment") | ||
| } |
Comment on lines
+73
to
+77
| // TestSetProcessDone_ErrorAvailableImmediately validates that getProcessError() | ||
| // returns the correct error immediately after processDone is closed. | ||
| // The current implementation copies the error in an async goroutine, which | ||
| // creates a race: the channel close is visible to callers before the error | ||
| // is stored, so getProcessError() can return nil. |
Comment on lines
+116
to
+119
| // TestSetProcessDone_RequestMissesProcessError validates that the Request() | ||
| // method can fall through to the generic "process exited unexpectedly" message | ||
| // when the SetProcessDone goroutine hasn't copied the error in time. | ||
| func TestSetProcessDone_RequestMissesProcessError(t *testing.T) { |
Comment on lines
+159
to
+162
| // TestSetProcessDone_ErrorCopiedEventually verifies that the error IS eventually | ||
| // available if we give the goroutine time to run — confirming the issue is | ||
| // purely a timing race, not a logic error. | ||
| func TestSetProcessDone_ErrorCopiedEventually(t *testing.T) { |
| return fmt.Errorf("failed to create stdout pipe: %w", err) | ||
| } | ||
|
|
||
| c.process.Stderr = &bytes.Buffer{} |
Comment on lines
1282
to
1292
| // For TCP mode, capture stdout to get port number | ||
| stdout, err := c.process.StdoutPipe() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create stdout pipe: %w", err) | ||
| } | ||
|
|
||
| c.process.Stderr = &bytes.Buffer{} | ||
|
|
||
| if err := c.process.Start(); err != nil { | ||
| return fmt.Errorf("failed to start CLI server: %w", err) | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix: Capture CLI stderr and resolve SetProcessDone race
Problem
When the CLI process exits during startup, the SDK reports an opaque error with no indication of why the process failed:
The actual cause (e.g. missing module, auth failure, version mismatch) is written to stderr, which the SDK silently discards.
Root Causes
client.gonever setsexec.Cmd.Stderr, so all CLI diagnostic output is lost.SetProcessDone—jsonrpc2.gocopies the process error inside an async goroutine. WhenRequest()detectsprocessDoneis closed and immediately callsgetProcessError(), the goroutine hasn't run yet, so the error isniland callers get the generic"process exited unexpectedly"message.Changes
client.goc.process.Stderr = &bytes.Buffer{}beforeStart()in both stdio and TCP branchesclient.gomonitorProcessafterWait()and include contents in errorinternal/jsonrpc2/jsonrpc2.goSetProcessDonewith direct error pointer storageinternal/jsonrpc2/jsonrpc2.gogetProcessError()now dereferences the stored pointer, which is set before channel closeclient_test.gointernal/jsonrpc2/jsonrpc2_test.goprocessDoneclosesAfter
Testing
TestMonitorProcess_StderrCaptured— stderr included in error on non-zero exitTestMonitorProcess_StderrCapturedOnZeroExit— stderr included on exit code 0TestStartCLIServer_StderrFieldSet— confirmsStderris assignedTestSetProcessDone_ErrorAvailableImmediately— no race: error accessible right after channel closeTestSetProcessDone_RequestMissesProcessError—Request()returns actual error, not generic messageTestSetProcessDone_ErrorCopiedEventually— sanity check that error pointer works after yieldManual Test Evidence
Before the fix — stderr is discarded, error is opaque:
After the fix — stderr is captured and included in the error:
The error now clearly shows the CLI failed because of a missing Node.js module, making the root cause immediately actionable.